Skip to content

Keep HTTPError as a base class for .request() and .raise_for_status()#1125

Merged
lovelydinosaur merged 1 commit intomasterfrom
httperror-base-class
Aug 3, 2020
Merged

Keep HTTPError as a base class for .request() and .raise_for_status()#1125
lovelydinosaur merged 1 commit intomasterfrom
httperror-base-class

Conversation

@lovelydinosaur
Copy link
Contributor

@lovelydinosaur lovelydinosaur commented Aug 3, 2020 •
edited
Loading

Closes #1108, and following rationale in #1083 (comment), which I'll repeat here...

I think there's probably also a decent case to make for not deprecating it at all, and instead keeping it as a base class, but only for RequestError and HTTPStatusError, so that...

try:
    response = httpx.get("https://www.example.com/")  # May raise a RequestError subclass
    response.raise_for_status()  # May raise HTTPStatusError
except httpx.HTTPError as exc:
    pass

Which means existing code all keeps working as expected, while still providing finer-grained classes where needed.

It's also more concise than the alternative...

try:
    response = httpx.get("https://www.example.com/")  # May raise a RequestError subclass
    response.raise_for_status()  # May raise HTTPStatusError
except (httpx.RequestError, httpx.HTTPStatusError) as exc:
    pass

This hadn't really occurred to me when looking at #1095.

The typing is also still nice and clear here. Both HTTPStatusError and RequestError provide .request, so we can suitably annotate that on the base class. However accessing .response on httpx.HTTPError would be a typing error, since it could either be a RequestError (which isn't associated with a response) or a HTTPStatusError (which is).

The following are all valid...

response = httpx.get("https://www.example.com/")  # May raise a RequestError subclass
try:
    response.raise_for_status()  # May raise HTTPStatusError
except httpx.HTTPStatusError as exc:
    print(f"Unexpected status code in response: {exc.response.status_code}")
try:
    response = httpx.get("https://www.example.com/")  # May raise a RequestError subclass
    response.raise_for_status()  # May raise HTTPStatusError
except httpx.RequestError as exc:
    print(f"Request to {exc.request.url} failed: {exc.message}")
except httpx.HTTPStatusError as exc:
    print(f"Unexpected status code in response: {exc.response.status_code}")
try:
    response = httpx.get("https://www.example.com/")  # May raise a RequestError subclass
    response.raise_for_status()  # May raise HTTPStatusError
except httpx.HTTPError as exc:
    if isinstance(exc, HTTPStatusError):
        # The following line will type-check properly now.
        print(f"Unexpected status code in response: {exc.response.status_code}")
    else:
        print(f"Request to {exc.request.url} failed: {exc.message}")

I think this is probably all a usability win for our users given...

  • except httpx.HTTPError as exc: is more concise than except (httpx.RequestError, httpx.StatusCodeError) as exc:
  • It's a closer match for requests codebases that are migrating.
  • We're not breaking anything for our existing users here.

How do we feel about this?

@lovelydinosaur lovelydinosaur added the enhancement New feature or request label Aug 3, 2020
@lovelydinosaur lovelydinosaur added this to the v0.14 milestone Aug 3, 2020
@iwoloschin
Copy link

iwoloschin commented Aug 3, 2020

This looks great, with one question, is there a reason you're explicitly not using HTTPError as a base exception for all possible httpx exceptions?

@lovelydinosaur
Copy link
Contributor Author

lovelydinosaur commented Aug 3, 2020 •
edited
Loading

is there a reason you're explicitly not using HTTPError as a base exception for all possible httpx exceptions?

Absolutely, yup. RequestError and HTTPStatusError are categories of exception that you may want to catch and handle.

The remaining cases are due to programmatic errors. It's not typically useful to catch and handle those cases, and including them in HTTPError means we'd be over-generalising a class that's really just intended for the particularly useful and common class of errors that are "could not make the request, or the response was 4xx/5xx".

Of the remaining cases...

  • NotRedirectResponse occurs if you're calling response.next() without properly checking .is_redirect_response first.
  • The StreamError cases occur if you're accessing request/response streams in some broken way, eg. trying to stream the body twice, trying to stream the body after having closed it etc.
  • CookieConflictError occurs if you need to be more specific in your response.cookies.get() access, by including domain=... and path=... arguments to narrow down which domain/path you want a cookie for. (Granted, this is a bit more ambiguous than the other two cases.)

@iwoloschin
Copy link

iwoloschin commented Aug 3, 2020

Ah, well, that explains why I've never really needed those other exceptions! I think that's a reasonable explanation and I'd recommend that something similar (or even just what you wrote) definitely be included in the documentation.

@lovelydinosaur lovelydinosaur mentioned this pull request Aug 3, 2020
Merged
florimondmanca
florimondmanca approved these changes Aug 3, 2020
View reviewed changes
Copy link
Contributor

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds about right to me! I'm also feeling we could keep this w/o deprecation, it's probably a nice handy feature (which I agree should be documented but I understand we haven't started documenting the exception hierarchy just yet).

@lovelydinosaur lovelydinosaur merged commit d7aa6e0 into master Aug 3, 2020
@lovelydinosaur lovelydinosaur deleted the httperror-base-class branch August 3, 2020 19:06
@michaeloliverx
Copy link
Contributor

michaeloliverx commented Feb 18, 2022

The .request attribute was removed from HTTPError in #1522, was this intended?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@florimondmanca florimondmanca florimondmanca approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

v0.14

Development

Successfully merging this pull request may close these issues.

Should HTTPError be a base class for RequestError and HTTPStatusError?

4 participants

@lovelydinosaur @iwoloschin @michaeloliverx @florimondmanca